New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CURATOR-594: TestingZooKeeperMain isn't setting tickTime, if configured #383
Conversation
6892faa
to
ee38cc6
Compare
The test isn't that elegant, but probably need mockito or some other mocking framework to get it tested, I'm opened to change it, if needed. |
final File tmpDir = testingServer.getTempDirectory(); | ||
tmpDir.deleteOnExit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear why we need this? Can you add a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with the server files and if there is a I/O crash con close the files won't be deleted
With junit I would have used temp files provided by the test suite itself, with jupiter I don't know :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with jupiter I don't know :(
there is support for temporary files as well
https://www.baeldung.com/junit-5-temporary-directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me handle this 👍
ee38cc6
to
55b9b85
Compare
@madrob the CI seems to pass with jdk 11, but not with jdk 8, but cannot say why, let me know if there is anything more I can do to help 👍 |
I have restarted the build on jdk8 |
@eolivelli I've had a chat with @Randgalt on https://issues.apache.org/jira/browse/CURATOR-595?focusedCommentId=17330211&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17330211 that's related (ie this fix was meant to allow me to use the server for my integration tests with curator & ZK), but it seems to fail regularly despite this fix vs the test provided in the issue I've linked above... |
55b9b85
to
9cb6e44
Compare
Fixed the tmp directory thanks to @eolivelli comment 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
Let me merge as soon as Travis passes |
@eolivelli any update on merging this? its been a month now.... |
Sorry. I forgot |
@eolivelli Thanks , what is roughly the timeline of the next release? Just as we're planning on using this in activemq artemis, but dependent on this fix for us to progress and ultimately ourselves release the feature. |
https://issues.apache.org/jira/browse/CURATOR-594